Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce ruff/refurb rules (FURB) #2373

Merged
merged 6 commits into from
Oct 19, 2024

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Oct 15, 2024

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good here. Just need to resolve the conflict.

pyproject.toml Outdated
"FURB140", # TODO: apply this rule
"PT004", # deprecated
"PT011", # TODO: apply this rule
"PT012", # TODO: apply this rule
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to do these here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean exactly? Don't you like the TODO comments, or the disabled rules? I've disabled them because I'm not necessarily happy with the readability of the code after applying FURB118 and FURB140.

Would it be OK to apply the rules, show the result, and let reviewers decide whether to keep the rules or disable them (without a TODO comment)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just curious if these TODOs are for this PR are a later one.

Copy link
Contributor Author

@DimitriPapadopoulos DimitriPapadopoulos Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me apply these rules now, I'm interested in your opinion.

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the FURB branch 3 times, most recently from 06af42d to c1b9760 Compare October 18, 2024 18:33
FURB110 Replace ternary `if` expression with `or` operator
FURB118 Use `operator.itemgetter(0)` instead of defining a lambda
FURB140 Use `itertools.starmap` instead of the generator
FURB188 Prefer `removesuffix` over conditionally replacing with slice.
@jhamman jhamman self-requested a review October 19, 2024 15:47
Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good here

@jhamman jhamman merged commit 9dd9ac6 into zarr-developers:main Oct 19, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants